[PATCH 2/3] term/serial: Reject caller-supplied port addresses under lockdown

The serial command is available in lockdown mode and the port option
accepts a raw port address or a raw mmio base address. This can be
misused to allow an attacker to write to local memory and do things
including disabling / bypassing secure boot as long as the
console is available or an unsigned GRUB configuration is under
the attacker's control.

Update the serial logic to only accept a raw address in lockdown when
a port is already registered at it, which means the platform described
that UART: grub_ns8250_init() for the legacy com<N> ports,
grub_pciserial_init() for an enumerated PCI device, or an ACPI SPCR
UART once "auto" has parsed the table. Names that are not addresses
are untouched, so "auto", efi<N> from grub_efiserial_init() - the
route on arm64, which has neither ns8250 nor SPCR support - and
lookups by registered name all keep working.

Behavior without lockdown (no secure boot) is unchanged.

Assisted-by: Claude:Opus-5
(Tweaked to fit into the 2.14 codebase by Steve McIntyre <93sam@debian.org>

Signed-off-by: Andrew Hamilton <adhamilt@gmail.com>
Suggested-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Reviewed-by: Leo Sandoval <lsandova@redhat.com>
Part-of: <https://gitlab.freedesktop.org/gnu-grub/grub/-/merge_requests/266>

Gbp-Pq: Topic upstream
Gbp-Pq: Name 0002-term-serial-Reject-caller-supplied-port-addresses-un.patch

diff --git a/grub-core/term/ns8250-spcr.c b/grub-core/term/ns8250-spcr.c
index 428b2d59a7d932cbc020c4031ed816f10576ed28..454946deff71013dfe0e46ab5496141b6a252cfc 100644 (file)
--- a/grub-core/term/ns8250-spcr.c
+++ b/grub-core/term/ns8250-spcr.c
@@ -83,11 +83,13 @@ grub_ns8250_spcr_init (void)
 
   switch (spcr->base_addr.space_id)
     {
+      /* The address comes from the platform's own ACPI table, so it is trusted. */
       case GRUB_ACPI_GENADDR_MEM_SPACE:
         return grub_serial_ns8250_add_mmio (spcr->base_addr.addr,
-                                            spcr->base_addr.access_size, &config);
+                                            spcr->base_addr.access_size, &config,
+                                            true);
       case GRUB_ACPI_GENADDR_IO_SPACE:
-        return grub_serial_ns8250_add_port (spcr->base_addr.addr, &config);
+        return grub_serial_ns8250_add_port (spcr->base_addr.addr, &config, true);
       default:
         return NULL;
     };

diff --git a/grub-core/term/ns8250.c b/grub-core/term/ns8250.c
index 23e8e09048a78faf73a00b62ca2c182b28cbdc62..014bb046112f3712ff7dca4c1c8434e2c2d59151 100644 (file)
--- a/grub-core/term/ns8250.c
+++ b/grub-core/term/ns8250.c
@@ -347,7 +347,8 @@ grub_ns8250_hw_get_port (const unsigned int unit)
 }
 
 struct grub_serial_port *
-grub_serial_ns8250_add_port (grub_port_t port, struct grub_serial_config *config)
+grub_serial_ns8250_add_port (grub_port_t port, struct grub_serial_config *config,
+                            bool trusted)
 {
   struct grub_serial_port *p;
   unsigned i;
@@ -363,14 +364,23 @@ grub_serial_ns8250_add_port (grub_port_t port, struct grub_serial_config *config
        return &com_ports[i];
       }
 
+  /*
+   * use_mmio and port share a union with the private data of every other back
+   * end, so they only mean anything on a port this driver registered.
+   */
   FOR_SERIAL_PORTS (p)
-    if (p->use_mmio == false && p->port == port)
+    if (p->driver == &grub_ns8250_driver
+       && p->use_mmio == false && p->port == port)
       {
         if (config != NULL)
           grub_serial_port_configure (p, config);
         return p;
       }
 
+  /* No port is registered here, so going on means probing the address. */
+  if (grub_serial_reject_untrusted_address (trusted) == true)
+    return NULL;
+
   grub_outb (0x5a, port + UART_SR);
   if (grub_inb (port + UART_SR) != 0x5a)
     return NULL;
@@ -402,7 +412,7 @@ grub_serial_ns8250_add_port (grub_port_t port, struct grub_serial_config *config
 
 struct grub_serial_port *
 grub_serial_ns8250_add_mmio (grub_addr_t addr, unsigned int acc_size,
-                             struct grub_serial_config *config)
+                             struct grub_serial_config *config, bool trusted)
 {
   struct grub_serial_port *p;
   unsigned i;
@@ -415,14 +425,23 @@ grub_serial_ns8250_add_mmio (grub_addr_t addr, unsigned int acc_size,
         return &com_ports[i];
       }
 
+  /*
+   * use_mmio and mmio share a union with the private data of every other back
+   * end, so they only mean anything on a port this driver registered.
+   */
   FOR_SERIAL_PORTS (p)
-    if (p->use_mmio == true && p->mmio.base == addr)
+    if (p->driver == &grub_ns8250_driver
+       && p->use_mmio == true && p->mmio.base == addr)
       {
         if (config != NULL)
           grub_serial_port_configure (p, config);
         return p;
       }
 
+  /* No port is registered here, so going on means programming the address. */
+  if (grub_serial_reject_untrusted_address (trusted) == true)
+    return NULL;
+
   p = grub_malloc (sizeof (*p));
   if (p == NULL)
     return NULL;

diff --git a/grub-core/term/serial.c b/grub-core/term/serial.c
index 8260dcb7a87a8775170b15c1e1f1e64eb4d72b23..57039dd716b1ac33de7036d1f60febe6dc83b06d 100644 (file)
--- a/grub-core/term/serial.c
+++ b/grub-core/term/serial.c
@@ -28,6 +28,7 @@
 #include <grub/extcmd.h>
 #include <grub/i18n.h>
 #include <grub/list.h>
+#include <grub/lockdown.h>
 #ifdef GRUB_MACHINE_MIPS_LOONGSON
 #include <grub/machine/kernel.h>
 #endif
@@ -140,6 +141,30 @@ static struct grub_term_output grub_serial_term_output =
 
 \f
 
+/*
+ * An address that no port is registered at cannot be honored while lockdown
+ * is enforced: the probe and programming sequences in the ns8250 and pl011
+ * back ends write to fixed offsets from it with no way to tell a UART from
+ * ordinary memory, so an address taken from an unauthenticated configuration
+ * is a write primitive against whatever it points at.
+ *
+ * A back end calls this once its own lookup has found no port already
+ * registered at the address, that is, at the point where it would otherwise
+ * touch the hardware. TRUSTED is true when the address came from a platform
+ * description -- grub_ns8250_init(), an ACPI SPCR table, PCI enumeration --
+ * rather than from the caller.
+ */
+bool
+grub_serial_reject_untrusted_address (bool trusted)
+{
+  if (trusted == true || grub_is_lockdown () != GRUB_LOCKDOWN_ENABLED)
+    return false;
+
+  grub_error (GRUB_ERR_ACCESS_DENIED,
+             N_("serial port address is not permitted when lockdown is enforced"));
+  return true;
+}
+
 struct grub_serial_port *
 grub_serial_find (const char *name)
 {
@@ -154,12 +179,18 @@ grub_serial_find (const char *name)
     if (grub_strcmp (port->name, name) == 0)
       return port;
 
+  /*
+   * Everything below this point builds a port from an address the caller
+   * gave us, so the back ends are told the address is untrusted: under
+   * lockdown they hand back a port only if one is already registered at that
+   * address, which means the platform described that UART.
+   */
 #if (defined(__mips__) || defined (__i386__) || defined (__x86_64__)) && !defined(GRUB_MACHINE_EMU) && !defined(GRUB_MACHINE_ARC)
   if (grub_strncmp (name, "port", sizeof ("port") - 1) == 0
       && grub_isxdigit (name [sizeof ("port") - 1]))
     {
       port = grub_serial_ns8250_add_port (grub_strtoul (&name[sizeof ("port") - 1],
-                                                       0, 16), NULL);
+                                                       0, 16), NULL, false);
       if (port != NULL)
         return port;
     }
@@ -205,7 +236,7 @@ grub_serial_find (const char *name)
             grub_error (GRUB_ERR_BAD_ARGUMENT, N_("incorrect MMIO access size"));
           }
 
-      port = grub_serial_ns8250_add_mmio (addr, acc_size, NULL);
+      port = grub_serial_ns8250_add_mmio (addr, acc_size, NULL, false);
       if (port != NULL)
         return port;
     }
@@ -275,9 +306,18 @@ grub_cmd_serial (grub_extcmd_context_t ctxt, int argc, char **args)
 
   port = grub_serial_find (name);
   if (!port)
-    return grub_error (GRUB_ERR_BAD_ARGUMENT,
-                      N_("serial port `%s' isn't found"),
-                      name);
+    {
+      /*
+       * A back end that refused the port has already said why; keep that
+       * rather than reporting a misleading "isn't found".
+       */
+      if (grub_errno == GRUB_ERR_ACCESS_DENIED)
+       return grub_errno;
+
+      return grub_error (GRUB_ERR_BAD_ARGUMENT,
+                        N_("serial port `%s' isn't found"),
+                        name);
+    }
 
   config = port->config;
 

diff --git a/include/grub/serial.h b/include/grub/serial.h
index d7e06357881558ae8b1290c472826ade42b5216c..096209aeb6570004414b5e0ce4ad31b81d48c8f7 100644 (file)
--- a/include/grub/serial.h
+++ b/include/grub/serial.h
@@ -188,14 +188,18 @@ grub_serial_config_defaults (struct grub_serial_port *port)
   return port->driver->configure (port, &config);
 }
 
+bool grub_serial_reject_untrusted_address (bool trusted);
+
 #if defined(__mips__) || defined (__i386__) || defined (__x86_64__)
 void grub_ns8250_init (void);
 struct grub_serial_port *grub_ns8250_spcr_init (void);
 struct grub_serial_port *grub_serial_ns8250_add_port (grub_port_t port,
-                                                     struct grub_serial_config *config);
+                                                     struct grub_serial_config *config,
+                                                     bool trusted);
 struct grub_serial_port *grub_serial_ns8250_add_mmio (grub_addr_t addr,
                                                      unsigned int acc_size,
-                                                     struct grub_serial_config *config);
+                                                     struct grub_serial_config *config,
+                                                     bool trusted);
 #endif
 #ifdef GRUB_MACHINE_IEEE1275
 void grub_ofserial_init (void);